added workflow start-update-with-start and workflow execute-update-with-start commands
#762
Conversation
|
Need to add tests, but feel free to take a look at the command interface and behaviour |
| var valuePtr interface{} | ||
| err = handle.Get(cctx, &valuePtr) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to update workflow: %w", err) | ||
| } |
There was a problem hiding this comment.
There's a bit of a problem with this approach and the one taken by temporal workflow update execute. This presumes that the result can be JSON decoded, but many people use protos or other formats. On user-defined results we need to support all payloads (gracefully using JSON where we can). I believe this is the first time we have needed to get a raw payload from a high-level client call to display to a user.
I think we need to add support for rawValuePayloadConverter.FromPayload in client.go, then change the code in this PR to be something like:
var raw RawValue
if err = handle.Get(cctx, raw); err != nil {
return fmt.Errorf("unable to update workflow: %w", err)
}
result, err := cctx.MarshalFriendlyJSONPayloads(raw.Payload)And have Result in the structure be a json.RawMessage. Same for temporal workflow update. And confirm it looks right even with -o json. Thoughts @THardy98 and @dandavison?
…rom printed output of commands because we cannot guarantee the task queue used is the one the user supplied
|
I removed Also refactored the |
| // Currently we only accept 'accepted' as a valid wait for stage value, but we intend | ||
| // to support more in the future. | ||
| if waitForStage == client.WorkflowUpdateStageAccepted { | ||
| // Use a canceled context to check whether the initial server response | ||
| // shows that the update has _already_ failed, without issuing a second request. | ||
| ctx, cancel := context.WithCancel(cctx) | ||
| cancel() | ||
| err = handle.Get(ctx, nil) | ||
| var timeoutOrCanceledErr *client.WorkflowUpdateServiceTimeoutOrCanceledError | ||
| if err != nil && !errors.As(err, &timeoutOrCanceledErr) { | ||
| return fmt.Errorf("unable to update workflow: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hrmm, I don't think UpdateWithStartWorkflow will return until it is at least accepted or rejected, and if there is some error, it will be reflected in the error to that call. @dandavison - is this correct?
There was a problem hiding this comment.
An example is sending a request for an update name that does not exist. err is nil, but the update is not accepted, and handle contains within it an error, hence this code (which is also present in CLI update impl) to extract that error without making a network request.
There was a problem hiding this comment.
Ah, I see, there is "accepted but failed". We should consider adding a Done() bool to WorkflowUpdateHandle for this, but in the meantime, all good here.
(EDIT: Found we do have an issue at temporalio/features#428)
|
TYFR :) |
added
temporal workflow start-update-with-startandtemporal workflow execute-update-with-startcommandstemporal workflow start-update-with-startusage:temporal workflow execute-update-with-startusage:Closes UpdateWithStart SDK API #664
How was this tested:
Yes